WIP/REF/FIX: Don't throw away exception stack traces #1490
Conversation
| @@ -459,9 +460,9 @@ def create_average_networks_by_group_workflow(group_list, data_dir, subjects_dir | |||
| try: | |||
| l4infosource.inputs.group_id1 = list(group_list.keys())[0] | |||
| l4infosource.inputs.group_id2 = list(group_list.keys())[1] | |||
| except IndexError: | |||
| except IndexError as e: | |||
| print('The create_average_networks_by_group_workflow requires 2 groups') | |||
There was a problem hiding this comment.
shouldn't we include this in the Exception message rather than print it?
There was a problem hiding this comment.
Yes, thanks for catching that.
|
This change unfortunately decreased test coverage significantly. To test my changes, I would like to use the auto-test-maker to define inputs that should induce exceptions and write tests, but I am not sure how or if that's possible. Thoughts? |
| except KeyError: | ||
| raise ImportError("This loader does not know module " + fullname) | ||
| except KeyError as e: | ||
| raise_from(ImportError("This loader does not know module " + fullname), e) |
There was a problem hiding this comment.
since this is an external package, we shouldn't make this change. it may also be time to update the six.py included in nipype.
There was a problem hiding this comment.
I see. Are all the files in external this way? If possible, we should avoid this
There was a problem hiding this comment.
we should update external files before a release, but any changes should really be upstream. six and future are competing packages and at present we are using both. it will require a focused sprint to use one or the other.
There was a problem hiding this comment.
got it, that makes sense
|
@shoshber - can you merge with upstream master? regarding the auto-test-makter, this wouldn't generally be covered under that. how, about testing a few basic test cases that would show error propagation? |
Conflicts: nipype/pipeline/plugins/ipython.py
Will definitely do that--doctest appears to have a pretty good builtin way to do it, I might try that |
berleant
changed the title
|
@shoshber - this is looking good. should we merge this or are you planning to add more tests? If you think I've added enough tests, then let's merge! |
| @@ -45,6 +45,7 @@ install: | |||
| - if [ ${TRAVIS_PYTHON_VERSION:0:1} == "2" ]; then conda install --yes vtk; fi | |||
| - pip install python-coveralls | |||
| - pip install nose-cov | |||
| - pip install mock | |||
There was a problem hiding this comment.
Shouldn't this be removed from here and added to https://github.com/nipy/nipype/blob/master/nipype/info.py? Same thing goes for circle.yml. (this way users will be able to run tests)
I found some places in the code where exceptions were being caught and a new exception being thrown without the stack trace or any other relevant info. This makes it hard to debug and figure out what errors mean. Python 3 introduced a
raise ... fromlanguage construct that makes it easier to chain exceptions and keep the stack trace/other relevant info. Thefuturemodule allows similar behavior in Python 2 with theraise_fromfunction, which I've used here.I would also like to propose adding a guideline for contributing to nipype with wording like (first draft, suggestions welcome): "In general, do not catch exceptions without good reason. Good reasons include catching non-fatal exceptions and adding more information about what may have caused the error. A caught exception must be either 1) rethrown inside a new exception using
future'sraise_from, or 2) logged. Do not do both (creates confusing output)."Another note (for a future PR?): We probably should not be using/catching the Exception class without subclassing it (per python documentation: https://docs.python.org/3/library/exceptions.html).